Use unique string IDs #142

CunliangGeng · 2023-05-12T13:40:57Z

This is a very big PR with a lot of changes. It's very important to keep in mind that the purpose is to use unique string ID for scoring.

Brief background:

GCF, Spectrum and MolecularFamily objects have integer and/or string IDs:
- GCF.id and GCF.gcf_id
- Spectrum.id and Spectrum.spectrum_id
- MolecularFamily.id and MolecularFamily.family_id
The string ID might not exist and are also not really used.
The integer IDs (*.id) are used for scoring part. But it caused a lot trouble because
- The value of these IDs are not unique and can change when the order of the objects is changed
  - e.g. in [gcf1, gcf2] gcf1.id is 0; but [gcf2, gcf1] gcf1.id will be 1.
- Scoring code use these IDs as the index to get the value from numpy array. However when the order of the objects is changed (the array does not change), it's totally wrong to use the new ID to select value from the array.

The solution is to only use string ID (unique) and remove the changeable integer ID.

Major Changes

change GCF.gcf_id, Specrum.spectrum_id and MolecularFamily.family_id to string ID
use string ID for scoring (DataLinks, LinkFinder, MetcalfScoring)
- replace np.array with pandas.DataFrame (use string ID as index/column names) to store score data
- update logics of code to use data frame and to use string ID to select values
remove the assignment of integer ID (*.id)
remove the use of integer ID (*.id) in StrainCollection
add docstrings and unit tests for scoring related classes
add TODO comments to files for further refactoring

Important but not involved in this PR

make sure all string ID of all objects are unique
remove *.id from all objects

The following BGC attributes are updated: - product_prediction - mibig_bgc_class - smiles

Parameters before "/" are positional-only parameters, see https://docs.python.org/3/glossary.html#term-parameter.

to figure out how `spectrum_id` is set

CunliangGeng · 2023-05-15T10:03:53Z

Ready for review ;-)
You could ignore the CI tests since there is an error in BigScape dependency and I have requested its maintainer to fix it.
All the unit tests have passed locally.

hechth

Awesome work! I'm just a bit confused by the testing of NPLinker scores and Metcalf scores - it seems like some of the tests are duplicated, or almost at least?

hechth · 2023-05-24T09:53:40Z

src/nplinker/genomics/genomics.py

-        strains.remove(bgc.strain)
+        if bgc.strain is not None:
+            strains.remove(bgc.strain)


What is the scenario where the strain object of the bgc is None?

The strain is None by default, so I guess this happens in all cases in which the strain is not set through the strain setter method

Giulia is right. The strain might be None when BGC instances are not created using BGC Loaders (in the loaders, the strain will be set).

src/nplinker/metabolomics/gnps/gnps_molecular_family_loader.py

src/nplinker/metabolomics/metabolomics.py

src/nplinker/metabolomics/singleton_family.py

src/nplinker/nplinker.py

src/nplinker/scoring/linking/data_links.py

src/nplinker/scoring/linking/link_finder.py

tests/scoring/test_metcalf_scoring.py

gcroci2

What a huge work! Great :D

I added few minor comments, and I checked the code starting from the tests and trying then to dive deep into the internals. Since I don't know those parts of the package well, I am missing something here and there but overall I got the scope of the PR and I tried to check stuff like type hinting, doc strings, and comments. As I already mentioned, maybe next time it would be good to go through a PR like this together before reviewing it.

src/nplinker/nplinker.py

tests/test_nplinker_local.py

tests/test_strain_collection.py

gcroci2 · 2023-06-08T08:48:28Z

src/nplinker/genomics/genomics.py

-        strains.remove(bgc.strain)
+        if bgc.strain is not None:
+            strains.remove(bgc.strain)


The strain is None by default, so I guess this happens in all cases in which the strain is not set through the strain setter method

src/nplinker/metabolomics/gnps/gnps_molecular_family_loader.py

src/nplinker/metabolomics/spectrum.py

CunliangGeng · 2023-06-20T09:55:40Z

Awesome work! I'm just a bit confused by the testing of NPLinker scores and Metcalf scores - it seems like some of the tests are duplicated, or almost at least?

One is testing NPLinker object, another is testing Metcalf object. Though some tests look similar, they are serving different purposes and testing different objects.

src/nplinker/metabolomics/singleton_family.py

src/nplinker/scoring/metcalf_scoring.py

CunliangGeng added 30 commits April 5, 2023 18:16

change BGC attributes type from list to tuple

742d4b5

The following BGC attributes are updated: - product_prediction - mibig_bgc_class - smiles

use positional-only parameter in BGC and GCF

f1cac59

Parameters before "/" are positional-only parameters, see https://docs.python.org/3/glossary.html#term-parameter.

update BGC's __eq__ and __hash__

18e77a6

update GCF's __eq__ and __hash__

da2cf41

Update gcf.py

2111e5b

update Spectrum's __eq__ and __hash__

0736476

update MolecularFamily __eq__ and __hash__

fda1208

Update molecular_family.py

0a7ccee

update Strain __eq__ and __hash__

6a373e1

update StrainCollection __eq__

33ed3c6

add TODO comments to ObjectLink

f6bae56

add parameter type check for add_alias

f81accf

add __contains__ to Strain class

53dad82

update lookup method of StrainCollection

45d8765

update __contains__ in StrainCollection

7cb75e0

remove from __eq__

f797ee5

update __eq__ logic for Strain

8a3fa1b

rename _strain_dict_id to _strain_dict_name in StrainCollection

8875400

add comments to get_common_strains

2f77539

add comments and rename variables for DataLinks

dae421d

add comment about met_only parameter

504862e

add todo comments to LinkFinder

8b160e6

add comments to GNPSSpectrumLoader

8e9557c

to figure out how `spectrum_id` is set

change Spectrum.spectrum_id from type int to str

9257e0b

update spec_dict

43f6d80

Update tests

7536e42

update __eq__ in MolecularFamily

a0cf97a

change MolecularFamily.family_id from type int to str

b53af96

add method has_strain to MolecularFamily

ca848fd

Update metcalf_scoring.py

583fa33

Fix mypy warnings as much as possible

6ee04e9

This was linked to issues May 12, 2023

Implement removing the strain also from self._lookup indices. [Bug] #90

Closed

remove _strain_dict_index from StrainCollection class #134

Closed

Use unique and fixed GCF identifier in scoring #103

Closed

CunliangGeng marked this pull request as ready for review May 12, 2023 14:33

check strain existence using strain dict

88f48b3

hechth reviewed May 25, 2023

View reviewed changes

gcroci2 approved these changes Jun 20, 2023

View reviewed changes

CunliangGeng mentioned this pull request Jun 20, 2023

Use UUID for singleton molecular family? #144

Closed

3 tasks

CunliangGeng added 7 commits June 21, 2023 12:03

change calculate abbrevation from "cal" to "calc"

27bcc2d

remove resolved TODO comment

ab5d025

move shared fixtures to conftest.py

e4a0b3e

remove unnecessary type hints

ade54a3

update docstrings for cooccurrences

78aa794

use uuid for singleton molecular families #144

09c049f

add TODO comment for GNPSLoader

1ba7106

CunliangGeng requested review from hechth and gcroci2 June 21, 2023 10:06

CunliangGeng linked an issue Jun 21, 2023 that may be closed by this pull request

Use UUID for singleton molecular family? #144

Closed

3 tasks

This was referenced Jun 26, 2023

ensure that IDs are unique #146

Closed

clean up loader and downloader #149

Merged

hechth approved these changes Jul 2, 2023

View reviewed changes

src/nplinker/metabolomics/singleton_family.py Show resolved Hide resolved

src/nplinker/scoring/metcalf_scoring.py Outdated Show resolved Hide resolved

update type hints for *args parameter

0f1352a

CunliangGeng merged commit bd35f65 into dev Jul 3, 2023
0 of 2 checks passed

CunliangGeng deleted the use_unique_string_id branch July 4, 2023 08:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use unique string IDs #142

Use unique string IDs #142

CunliangGeng commented May 12, 2023

CunliangGeng commented May 15, 2023 •

edited

Loading

hechth left a comment

hechth May 24, 2023

gcroci2 Jun 8, 2023

CunliangGeng Jun 20, 2023

gcroci2 left a comment

gcroci2 Jun 8, 2023

CunliangGeng commented Jun 20, 2023

Use unique string IDs #142

Use unique string IDs #142

Conversation

CunliangGeng commented May 12, 2023

Brief background:

Major Changes

Important but not involved in this PR

CunliangGeng commented May 15, 2023 • edited Loading

hechth left a comment

Choose a reason for hiding this comment

hechth May 24, 2023

Choose a reason for hiding this comment

gcroci2 Jun 8, 2023

Choose a reason for hiding this comment

CunliangGeng Jun 20, 2023

Choose a reason for hiding this comment

gcroci2 left a comment

Choose a reason for hiding this comment

gcroci2 Jun 8, 2023

Choose a reason for hiding this comment

CunliangGeng commented Jun 20, 2023

CunliangGeng commented May 15, 2023 •

edited

Loading